Skip to content

Conversation

@danieldegrasse
Copy link
Contributor

@danieldegrasse danieldegrasse commented Jan 13, 2022

This PR is based off #41448, with the below differences:

  • SOC level nodes are used to define each pinctrl option, and overrides are provided at the board level
  • pinctrl properties renamed to align with pinctrl node properties in zephyr (where possible)

As with #41448, the nxp lpuart and i2s drivers are enabled. This PR was tested with the shell example.

@github-actions
Copy link

github-actions bot commented Jan 13, 2022

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_nxp zephyrproject-rtos/hal_nxp@aee8eaa (master) zephyrproject-rtos/hal_nxp#132 zephyrproject-rtos/hal_nxp#132/files

Note: This message is automatically posted and updated by the Manifest GitHub Action.

hakehuang
hakehuang previously approved these changes Jan 14, 2022
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution, please check my comments

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not standard property input-enable? Btw, this property is behaving as a boolean, so should be bool, and naming should use dashes, not underscores.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, use: bias-pull-down. List of options seems to only list a single pull-down resistor value.

Copy link
Contributor

@hakehuang hakehuang Jan 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in RT the two options is set to one set of bits, IMHO, keep the naming stupid and simple is better, rather than to fit to an enforced rules which just make the code hard to read.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This topic was largely discussed and was done for a reason: standardize as much as possible (remember Zephyr is multiplatform) and to align with other projects (Linux) that already have gone through this problem. I'm afraid we can't now make exceptions because a vendor doesn't like it. Of course, you can always take the time to make an alternative proposal if you think the current solution needs to be improved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may input-schmitt-enable be suitable? Again, it should be a boolean

Copy link
Contributor

@hakehuang hakehuang Jan 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as user, when I see below two options

      input-schmitt-enable:
        required: false
        type: boolean
        description: enable schmitt-trigger mode

      input-schmitt-disable:
        required: false
        type: boolean
        description: disable schmitt-trigger mode

I am quite confused on this, and I prefer to keep this as RT user manual named so.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

standard drive-strength can be used (you can customize it as ST/GD does for slew-rate)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is already customried, and no other platfrom can reuse this rule. I would rather to have NXP dedicated naming, if it is only apply to NXP platform. but leave @danieldegrasse to decide.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

standard drive-open-drain can be used

Comment on lines +235 to +241
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: if you make CONFIG_PINCTRL=y default, I think DT node should also be defaulted to okay

Comment on lines 15 to 23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: consider defaulting properties to SoC defaults, so that only required fields have to be defined. In many cases, only a few properties need to be adjusted (at least this is the case for ST/GD/Nordic)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this possible with pinctrl groups? With nodes like ST uses you can define the majority of the overrides at the SoC level, but with pinctrl groups the definition of the pinctrl node is placed within a board specific group. Default groups could be created for each SOC enabled within Zephyr, but that seems like it would deviate from the two pinctrl methods explained in Zephyr's documentation.

Of course, there's also the possibility that pinctrl nodes like ST uses are the preferred method. If that's the case, let me know and I can rebuild this PR to use that approach.

Copy link
Member

@gmarull gmarull Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By defaults I mean defaults in the binding file. If set to SoC defaults, then properties resolve to those values if not defined. For example, assume SoC default for nxp,speed is 2, then just set that property to 2 as default so that you don't need to define it for every group unless it has to be changed to another value.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can set a "sane" default for in the binding file, but different pins on RT parts have different SOC level configurations at boot, so some pins would get incorrect defaults assigned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also have multiple compatibles if parts differ (already happens in some other places, e.g. ST PLL nodes, you can have st,stm32f4xx-pll-clock.

@gmarull gmarull mentioned this pull request Jan 14, 2022
29 tasks
@zephyrbot zephyrbot added area: UART Universal Asynchronous Receiver-Transmitter area: Build System labels Jan 14, 2022
@danieldegrasse
Copy link
Contributor Author

This topic was largely discussed during pinctrl implementation, and the agreement was to respect this rule to standardize as much as possible.

Fair point, I will update this PR to use standard names were possible. Given how important SOC defaults appear to be to using pinctrl, I may end up switching this PR to use pinctrl nodes instead of groups

@gmarull
Copy link
Member

gmarull commented Jan 15, 2022

This topic was largely discussed during pinctrl implementation, and the agreement was to respect this rule to standardize as much as possible.

Fair point, I will update this PR to use standard names were possible. Given how important SOC defaults appear to be to using pinctrl, I may end up switching this PR to use pinctrl nodes instead of groups

In such case you may require them to be set explicitely, or, provide nodes with SoC defaults. Be aware of this when pre-defining "defaults" (ST has some issues due to that, like enabling pull-up for I2C by default):

image

@danieldegrasse
Copy link
Contributor Author

In such case you may require them to be set explicitly, or, provide nodes with SoC defaults. Be aware of this when pre-defining "defaults" (ST has some issues due to that, like enabling pull-up for I2C by default):

Thanks for the heads up- my only goal with defining defaults is to ensure that if a user doesn't override any properties on an pin node, it will not have the register value changed from what it is at startup. I see how defaulting to pull up for i2c without directly documenting it could cause issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this is conditionally included.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific ifdef is not required, although it does reduce the total include dependencies when pinctrl is disabled. The remaining ifdefs in the driver are required, but do you think this one should be removed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are transitioning to PINCTRL why should we provide an non-PINCTRL option?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long term, we should not. Short term, we need to to allow pinctrl to be conditional because boards that do not support it will fail to build using the driver if it requires pinctrl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as you don't have all drivers for the given SoC converted from pinmux to pinctrl, you should keep #ifdefs in the drivers. Otherwise, end-users will end up using a hybrid pinmux/pinctrl solution, where they need to investigate the source code of each enabled driver to see if they need to it up using pinctrl dts or pinmux.c.

@danieldegrasse
Copy link
Contributor Author

@gmarull I've rewritten the driver to use standard properties as much as possible, and moved the pinctrl system to use pinctrl nodes defined in NXP's HAL (same approach ST takes). The nodes in NXP's HAL have default properties assigned that correspond to the reset value of the register corresponding to the pin that pinctrl node configures. The only deviation from the standard naming I've applied is overriding bias-pull-up to be an enum, to allow the user to configure one of the several pull up resistors NXP supports on each pin.

@hakehuang I think I've captured all the hardware features we support in this implementation- if you look at the documentation in the devicetree yaml binding file it calls out what registers are affected by each pinctrl property. Could you let me know if you think there are additional features of NXP's IP that I'm not capturing here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it was present for future use within the DTS, but it is not needed now. I've removed it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use PINCTRL_STATE_DEFAULT for state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar addition can be made to the uart yaml file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't make an addition like this to to the uart yaml file until all boards supporting the uart driver also support pinctrl. If we add something like this, it effectively means all in tree boards with the kinetis-lpuart peripheral need to support pinctrl

This is because without defined pinctrl nodes for an LPUART node, adding these properties will cause the build to fail. It's the same issue that we have with using the #ifdef CONFIG_PINCTRL guards in the actual driver. In the case of the i2s driver, all boards that use it must support pinctrl, so these properties are fine and the ifdef guards aren't needed.

danieldegrasse and others added 11 commits January 24, 2022 11:50
NXP hal will define constants for pin mux options in RT pinctrl
implementation. Update hal revision to pull in header file.

Signed-off-by: Daniel DeGrasse <[email protected]>
add dts binding dedicated for mcux iomuxc settings

Signed-off-by: Hake Huang <[email protected]>
enable pinctrl in i2s and lpuart driver dts

Signed-off-by: Hake Huang <[email protected]>
add pinctrl_soc required headers

Signed-off-by: Hake Huang <[email protected]>
Signed-off-by: Daniel DeGrasse <[email protected]>
select pinctrl by default in rt series

Signed-off-by: Hake Huang <[email protected]>
add dtsi settings for rt series
dtsi use gpr to replace pinmux
nxp iomuxc has gpr which has more settings than mux and io settings
current solution is to export gpr separately and access then directly

Signed-off-by: Hake Huang <[email protected]>
Signed-off-by: Daniel DeGrasse <[email protected]>
add pinctrl driver for rt1xxx

Signed-off-by: Hake Huang <[email protected]>
enable pinctrl in lpuart

Signed-off-by: Hake Huang <[email protected]>
enable i2s pinctrl

Signed-off-by: Hake Huang <[email protected]>
enable pin control in board level

Signed-off-by: Hake Huang <[email protected]>
Signed-off-by: Daniel DeGrasse <[email protected]>
rename pinmux to gpr
different from pinmux and io settings gpr will do more IO
settings.

Signed-off-by: Hake Huang <[email protected]>
@hakehuang
Copy link
Contributor

think I've captured all the hardware features we support in this implementation- if you look at the documentation in the devicetree yaml binding file it calls out what registers are affected by each pinctrl property. Could you let me know if you think there are additional features of NXP's IP that I'm not capturing here?

I do not find any missing, your code is comprehensive.

@SebastianBoe SebastianBoe removed their request for review February 16, 2022 15:46
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this -- a few minor DTS things from me.

configuration values to reset values for the SOC register. Individual
overrides to pinctrl nodes can be applied at the board level.

for example, here is an override for the GPIO_AD_B0_12 pad:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: missing capitalization at beginning of sentence

slew-rate = <1>;
}

This will select GPIO_AD_B0_12 for use as SCL on LPI2C4, and enable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will it? It naively looks to me like this is configuring the behavior if that GPIO is used as SCL, but it doesn't seem to be actually selecting that pin for this purpose. Is this line correct? Just checking.

drive-strength:
required: false
type: int
default: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All defaults must be justified. This has no justification. Please fix it.

https://docs.zephyrproject.org/latest/guides/dts/bindings.html#rules-for-mainline-bindings

bias-pull-up:
required: false
type: int
default: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing justification

slew-rate:
required: false
type: int
default: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing justification

nxp,speed:
required: false
type: int
default: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing justification

* Copyright (c) 2022, NXP
* SPDX-License-Identifier: Apache-2.0
*
* Generated by rt_cfg_utils.py on 2022-01-19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need this line? If it's going into the boards directory like this, I guess it will be maintained by hand from now on; it's not clear why the comment that it was generated is relevant.

@danieldegrasse
Copy link
Contributor Author

@mbolivar-nordic Thanks for the feedback- after some internal discussion we are likely going to use a pin group approach instead of pin node based, as we feel it offers better flexibility with pin states, and aligns better with our linux pinctrl implementation. The PR capturing that implementation is #42238. I'm going to close this PR to avoid any further confusion, sorry about that!

@mbolivar-nordic
Copy link
Contributor

The PR capturing that implementation is #42238. I'm going to close this PR to avoid any further confusion, sorry about that!

No worries, thanks for the clarification! I'll wait for that to come out of draft before reviewing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: API Changes to public APIs area: Boards area: Build System area: Devicetree Binding PR modifies or adds a Device Tree binding area: Devicetree area: I2S area: Modules area: UART Universal Asynchronous Receiver-Transmitter DNM This PR should not be merged (Do Not Merge) manifest manifest-hal_nxp platform: NXP NXP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants